Adding snapshotting functionality for `MergingTree` #602

recursion-ninja · 2025-02-28T20:26:08Z

Adding snapshotting functionality for `MergingTree`

Summary

This PR contains the snapshotting functionality for the MergingTree data-type. In order to create a snapshot of a MergingTree, additional prerequisite functionality is also included:

Serialization via Encode.
Deserialization via DecodeVersioned.

As with creating the "metadata" snapshot, Intermediate "Snap-" types are created to media the conversion between a disk-representable format and the in-memory representation.

This PR includes test cases for:

The new Snap- data-types's Arbitrary instances.
Golden tests for the SnapshotMergingTree data to detect breakage of the format's encoding.
Property tests for "round-tripping" SnapshotMergingTree in memory
Property tests for "round-tripping" SnapshotMergingTree to and from disk via fs-api.

Potential issues:

While the SnapshotMergingTree data successfully round-trips to/from disk, I am not certain that conversion from a SnapshotMergingTree back to the in-memory representation of MergingTreeis correct. The Database.LSMTree.Internal.Snapshot.fromSnapMergingTree function defines this functionality, but I'm not sure I understand the usage of Ref and Refcounter well enough to ensure that the conversion is done correctly. Careful review of this function and/or suggestions for testing this conversion.
The functionality needs to be integrated into the Database.LSMTree.Internal.createSnapshot function after the questions below is resolved.
Currently, the MergingTree data snapshots are separate from the SnapshotMetadata, with each data-type having their own duplicate functions:
- writeFileSnapshotMergingTree
- readFileSnapshotMergingTree
- encodeSnapshotMergingTree
- decodeSnapshotMergingTree
The functionality for creating snapshots of the MergingTree data-type was created separately for ease of debugging and testing. However, now that the functionality is stable, this separation may be undesirable. It appears that createSnapshot is an "atomic" operation for the whole Table and any MergingTree within the Table should be serialized together within the SnapshotMetadata.

Questions

Should we have the SnapshotMergingTree be an entry within the SnapshotMetadata record, effectively combining the two types?
Does every use case for creating a SnapshotMetadata snapshot necessitate snapshots of any MergingTree(s)?
Does there exist a use case to snapshot a MergingTree without also creating a SnapshotMetadata` snapshot?

dcoutts

This is looking good.

I think we should rebase this after #588 since there's some related churn to the run/merging run/incoming run types and code.

We need to review the reference handling pattern. It should be callee duplicates when it retains references not caller. We should avoid functions that consume a reference / take ownership of a reference.

And I think we should extend this within the same PR to include the integration into the top level src/Database/LSMTree/Internal.hs, since otherwise we're not able to integrate it into the SnapshotMetaData type as we expect to do.

src/Database/LSMTree/Internal/MergingTree.hs

src/Database/LSMTree/Internal/Snapshot.hs

src/Database/LSMTree/Internal/Snapshot/Codec.hs

dcoutts · 2025-03-03T16:41:14Z

Rebased on #588.

dcoutts · 2025-03-04T00:20:33Z

src/Database/LSMTree/Internal/Snapshot.hs

+      -> SnapMergingRunState t (Ref (Run m h))
+      -> m (Ref (MR.MergingRun t m h))
+    getSnapMergingRunState nr md mc smrs = do
+          let ln = LevelNo 0


Hmm. This is fishy. We should figure out proper level numbers. Perhaps that means we need to include them in the snapshot.

So this problem has "moved" from the old call site, to the definitions of

toSnapMergingTreeState

toSnapPreExistingRun

The thing is, I don't know where to get the "correct" LevelNo from. The two call sites supplying LevelNo 0 to the getSnapMergingRunState function involve converting the MergingTreeState and PreExistingRun constructors:

SnapOngoingTreeMerge

SnapPreExistingMergingRun

I can't find any reasonable place form within the MergingTree data-type to extract a LevelNo from. Should the supplied LevelNo for the MergingTree be one grater than the number of Levels in the table?

Actually, after rebasing on main the problem site has moved again. The problem now manifests in fromSnapMergingRunState and is annotated with a TODO.

Actually, after rebasing on main the problem site has moved again. The problem now manifests in fromSnapMergingRunState and is annotated with a TODO.

Is this still a live issue? I don't see the TODO on fromSnapMergingRunState. And in the latest #608 there is no need for a LevelNo in fromSnapMergingRunState. So I hope this is now resolved.

Sorry, I wasn't clear enough about this. So in fromSnapMergingRunState there is this definition:

--TODO: the threshold should be stored with the MergingRun -- here we want to supply the credits now, so we can use a threshold of 1 let thresh = MR.CreditThreshold (MR.UnspentCredits 1) _ <- MR.supplyCreditsAbsolute mr thresh mergeCredits return mr

The "correct" way to create a CreditThreshhold, from what I can intuit from the exposed APIs, is to call Database.LSMTree.Internal.MergeSchedule.creditThresholdForLevel, and that function requires a LevelNo. That is what I meant by saying that the problem call site has just moved to the fromSnapMergingRunState definition. Is there another, better way to set the CreditThreshhold?

recursion-ninja · 2025-03-06T22:01:07Z

I believe that, at long last, this is ready to merge.

dcoutts · 2025-03-06T23:55:49Z

Looks like a minor rebase mistake (one of my patches appears twice, the second with minimal content). Removing this second one still works fine so I'll just force push it.

recursion-ninja · 2025-03-07T13:57:11Z

Removing this second one still works fine so I'll just force push it.

Thanks for doing that!

recursion-ninja · 2025-03-07T16:52:23Z

Phew, I finally got that rebase with main completed to resolve the merge conflicts.

… merge

Use utils for maybe, list and vector. And use a single-item encoding for Maybe, rather than it being a pair of things. The pair style means one has to remember to count it as two when setting the tuple lengths.

recursion-ninja requested review from dcoutts, jorisdral, mheinzel and wenkokke as code owners February 28, 2025 20:26

recursion-ninja force-pushed the recursion-ninja/merge-tree-serialize branch from 3d07f52 to e5acb06 Compare February 28, 2025 20:48

dcoutts reviewed Mar 3, 2025

View reviewed changes

dcoutts force-pushed the recursion-ninja/merge-tree-serialize branch from e5acb06 to 3acc5da Compare March 3, 2025 16:40

dcoutts changed the base branch from main to dcoutts/merge-credits-scaling-impl March 3, 2025 16:47

Base automatically changed from dcoutts/merge-credits-scaling-impl to main March 3, 2025 17:32

dcoutts reviewed Mar 4, 2025

View reviewed changes

recursion-ninja force-pushed the recursion-ninja/merge-tree-serialize branch 2 times, most recently from 600b813 to 94d9211 Compare March 6, 2025 22:00

dcoutts force-pushed the recursion-ninja/merge-tree-serialize branch from 94d9211 to 837e891 Compare March 6, 2025 23:55

recursion-ninja enabled auto-merge March 7, 2025 16:22

recursion-ninja force-pushed the recursion-ninja/merge-tree-serialize branch from 22e9d5a to 52899f1 Compare March 7, 2025 16:51

recursion-ninja disabled auto-merge March 7, 2025 16:57

dcoutts force-pushed the recursion-ninja/merge-tree-serialize branch from 52899f1 to ae3d37f Compare March 7, 2025 18:44

dcoutts changed the base branch from main to dcoutts/snapshot-refactoring March 7, 2025 18:45

dcoutts and others added 3 commits March 7, 2025 21:48

fixup test roundtrip on snapshot run

c78dd9a

Adding MertingTree to the snapshot functions, when present in a union…

f1857f3

… merge

Make the snapshot codec CBOR encoding more regular.

ca1a591

Use utils for maybe, list and vector. And use a single-item encoding for Maybe, rather than it being a pair of things. The pair style means one has to remember to count it as two when setting the tuple lengths.

dcoutts force-pushed the recursion-ninja/merge-tree-serialize branch from e1fbf2a to ca1a591 Compare March 7, 2025 21:48

dcoutts merged commit 1655f32 into dcoutts/snapshot-refactoring Mar 7, 2025

dcoutts deleted the recursion-ninja/merge-tree-serialize branch March 7, 2025 21:49

Adding snapshotting functionality for MergingTree #602

Adding snapshotting functionality for MergingTree #602

Uh oh!

Conversation

recursion-ninja commented Feb 28, 2025

Adding snapshotting functionality for MergingTree

Summary

Potential issues:

Questions

Uh oh!

dcoutts left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dcoutts commented Mar 3, 2025

Uh oh!

dcoutts Mar 4, 2025

Choose a reason for hiding this comment

Uh oh!

recursion-ninja Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

recursion-ninja Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

dcoutts Mar 7, 2025

Choose a reason for hiding this comment

Uh oh!

recursion-ninja Mar 7, 2025

Choose a reason for hiding this comment

Uh oh!

recursion-ninja commented Mar 6, 2025

Uh oh!

dcoutts commented Mar 6, 2025

Uh oh!

recursion-ninja commented Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

recursion-ninja commented Mar 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Adding snapshotting functionality for `MergingTree` #602

Adding snapshotting functionality for `MergingTree` #602

Adding snapshotting functionality for `MergingTree`

recursion-ninja commented Mar 7, 2025 •

edited

Loading